Summary

This PR optimizes GetInternal (previously BuildEligibleEventObjects) by using ValueListBuilder to reduce allocations when building the list of eligible event objects.

Critical Issues

1. Merge Conflict with PR #4308 🚨

This PR is based on code before PR #4308 was merged. PR #4308 (commit 8b189a0) completely restructured TestContextExtensions.cs to add EnsureEventReceiversCached and move BuildEligibleEventObjects to line 165.

This PR will revert all of PR #4308's work if merged as-is.

Required action: Rebase this PR on the latest main branch and re-apply the ValueListBuilder optimization to the current BuildEligibleEventObjects function (now at line 165).

2. Unsafe Cast to List

var attributes = (List<Attribute>)testContext.Metadata.TestDetails.GetAllAttributes();

This cast is brittle and relies on internal implementation details. GetAllAttributes() returns IReadOnlyList<Attribute>, which currently happens to be a List<Attribute> but could change.

Why it matters: The PR author notes this avoids List<Attribute>.Enumerator allocation, but this is a minor optimization (~40 bytes) that introduces significant risk. If the implementation ever returns an array or other IReadOnlyList implementation, this will throw InvalidCastException at runtime.

Suggested fix: Remove the cast and iterate over IReadOnlyList<Attribute> directly. The enumerator allocation is negligible compared to the risk.

Suggestions

1. Capacity calculation includes all items: The capacity pre-calculation (3 + testClassArgs.Length + ...) includes nulls, but AppendIfNotNull filters them out. This can lead to over-allocation. However, this is likely acceptable since ValueListBuilder uses pooled arrays, and slight over-estimation is better than reallocation.

2. injectedProps.Count > 0 check: The check if (injectedProps.Count > 0) to avoid enumerator allocation is good, but consider whether this is a hot enough path to warrant the extra branching vs. just iterating an empty collection.

Verdict

⚠️ REQUEST CHANGES - Critical merge conflict with PR #4308 and unsafe cast must be resolved.

Sorry, I had made this change a while back but was waiting to #4310.

IMO this is easier to read than #4307 and faster as this doesn't enumerate each collection twice, nor does it allocate the enumerators for List and IDictionary twice.

2. Unsafe Cast to List

I agree this isn't fantastic, I've added a Debug.Assert but I can remove this change if needed. I did think #4310 could change the type signature to a List but that might be confusing as it shouldn't be mutated.

Summary

This PR optimizes BuildEligibleEventObjects by using ValueListBuilder to reduce allocations.

Critical Issues

1. Complete Rewrite - Will Revert PR #4308

This PR completely deletes TestContextExtensions.cs (331 lines with caching infrastructure from PR #4308) and replaces it with 61 lines. This removes all the event receiver caching added in PR #4308.

Required: Rebase on latest main and apply ValueListBuilder optimization only to BuildEligibleEventObjects (line 165), keeping all caching logic intact.

2. Unsafe Cast to List

The cast (List)GetAllAttributes() is fragile. GetAllAttributes returns IReadOnlyList which happens to be a List today, but could change. The micro-optimization (~40 bytes) is not worth the brittleness risk.

Suggested fix: Remove cast, iterate over IReadOnlyList directly.

Previous Review Status

Maintainer already identified merge conflict. Author acknowledged waiting to rebase.

Verdict

REQUEST CHANGES:

1. Rebase on latest main to preserve PR perf: pre-compute and cache typed event receivers #4308 caching infrastructure
2. Remove unsafe cast to List

Note: The ValueListBuilder optimization itself is sound and will be beneficial once applied to the correct version.